Checked node condition for DaemonSets when updating node. #45649

k82cn · 2017-05-11T09:02:41Z

Which issue this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged): fixes #45628

Release note:

k8s-reviewable · 2017-05-11T09:02:47Z

This change is

0xmichalis · 2017-05-11T10:52:04Z

Do we want to requeue on all condition updates?

lukaszo · 2017-05-11T11:24:18Z

I think we should. It shouldn't happen to often.

0xmichalis · 2017-05-11T11:32:36Z

pkg/controller/daemon/daemoncontroller.go

+
+	curCond, err := v1helper.GetAvailableNodeConditionType(curNode.Status.Conditions)
+	if err != nil {
+		glog.Errorf("Failed to get current node's condition: %v", err)


I don't think there is any reason for having this error. The helper should return nil if there is no condition.

janetkuo · 2017-05-11T18:23:29Z

pkg/api/v1/helper/helpers.go

@@ -498,3 +498,14 @@ func PersistentVolumeClaimHasClass(claim *v1.PersistentVolumeClaim) bool {

 	return false
 }
+
+// GetAvailableNodeCondition returns the available NodeCondition of node.
+func GetAvailableNodeCondition(conds []v1.NodeCondition) *v1.NodeCondition {


This returns the first node condition whose status is true.

hm, got that. there'll be multiple condition, e.g. Disk, memory, right?

janetkuo · 2017-05-11T18:30:04Z

pkg/controller/daemon/daemoncontroller.go

+
+	nodeConditionTypeEqual := (oldCond == nil && curCond == nil)
+	if oldCond != nil && curCond != nil {
+		nodeConditionTypeEqual = (oldCond.Type == curCond.Type)


If the first node condition is true, all other node condition updates will be ignored.

janetkuo · 2017-05-11T18:35:32Z

@k82cn thanks for the fix! Would you add unit tests too?

k82cn · 2017-05-12T01:47:00Z

sure, will add a unit test for that.

janetkuo · 2017-05-12T20:50:30Z

pkg/controller/daemon/daemoncontroller.go

+
+	if reflect.DeepEqual(oldNode.Labels, curNode.Labels) &&
+		reflect.DeepEqual(oldNode.Spec.Taints, curNode.Spec.Taints) &&
+		v1helper.NodeConditionTypeEqual(oldCond, curCond) {


Just compare node conditions. No need to filter True ones before comparing.

Done; I filter by True with NodeConditionEqual, if Status is False or empty, ignore it.

janetkuo · 2017-05-12T20:53:28Z

pkg/api/v1/helper/helpers.go

+
+// NodeConditionTypeEqual returns true if all condition's status & types equals;
+// otherwise, returns false.
+func NodeConditionTypeEqual(c1 []v1.NodeCondition, c2 []v1.NodeCondition) bool {


nit: rename to NodeConditionsEqual

k82cn · 2017-05-13T08:19:24Z

@k8s-bot kops aws e2e test this

k82cn · 2017-05-13T08:30:25Z

@k8s-bot pull-kubernetes-federation-e2e-gce test this

lukaszo · 2017-05-14T10:02:06Z

pkg/api/v1/helper/helpers.go

+		return true
+	}
+
+	if len(c1) != 0 || len(c2) != 0 {


What's the goal of this if?

wow, this's used when filtered status is true condition before. It should be removed.

lukaszo · 2017-05-14T10:02:35Z

pkg/controller/daemon/daemoncontroller_test.go

@@ -1219,6 +1219,19 @@ func TestUpdateNode(t *testing.T) {
 			ds:            newDaemonSet("ds"),
 			shouldEnqueue: true,
 		},
+		{


Please add tests for all use cases.

Added a test when some conditions are false; in this PR, user's case is about out of disk but our logic does not tight to it.

I'll open another PR for resources changed, not solution for that case now.

janetkuo · 2017-05-17T21:51:58Z

/assign @lavalamp for approval

janetkuo · 2017-05-18T00:50:12Z

/assign @lavalamp

k82cn · 2017-05-18T00:59:04Z

@lavalamp , added a helper func NodeConditionEqual in v1/helper :). Do you have any comments.

k82cn · 2017-05-23T05:17:56Z

@k8s-bot pull-kubernetes-federation-e2e-gce test this

k82cn · 2017-05-24T02:09:18Z

@lavalamp , ping for approve & lgtm :).

lavalamp · 2017-05-24T19:36:33Z

pkg/api/v1/helper/helpers.go

@@ -524,3 +524,28 @@ func StorageNodeAffinityToAlphaAnnotation(annotations map[string]string, affinit
 	annotations[v1.AlphaStorageNodeAffinityAnnotation] = string(json)
 	return nil
 }
+
+// NodeConditionEqual returns true if all effective types ("Status" is true) equals;


This isn't what I'd expect an "equal" function to do?

hm, I think this's "equal" for the two node condition; if we want an exactly equal, we can use DeepEqual for all fields, including timestamps.

@lavalamp , is that OK for you? I'd like to get this merged before 1.7 cuts :).

@k82cn Not just that, only the ones whose status is true is compared. This is fine because we only care about node condition updates from true to non-true (either false or unknown). I suggest renaming it to NodeInSameContion, change c1 & c2 to old & cur, and move it to be a private function under daemonset controller code.

@janetkuo , thanks for the comments, updated :).

k82cn · 2017-06-01T05:20:46Z

@k8s-bot pull-kubernetes-unit test this
@k8s-bot pull-kubernetes-federation-e2e-gce test this

lukaszo · 2017-06-01T08:50:17Z

@k8s-bot pull-kubernetes-unit test this
@k8s-bot pull-kubernetes-federation-e2e-gce test this

k82cn · 2017-06-01T09:55:36Z

@k8s-bot pull-kubernetes-unit test this

k82cn · 2017-06-01T11:09:57Z

ping for lgtm label :).

lukaszo · 2017-06-01T11:52:48Z

/lgtm

k8s-github-robot · 2017-06-01T11:53:09Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: janetkuo, k82cn, kargakis, lukaszo

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these OWNERS Files:

~~pkg/controller/daemon/OWNERS~~ [janetkuo]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

k82cn · 2017-06-01T12:41:43Z

@k8s-bot pull-kubernetes-unit test this

k8s-github-robot · 2017-06-01T13:24:18Z

Automatic merge from submit-queue

k82cn · 2018-09-02T03:42:23Z

/sig apps

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label May 11, 2017

k8s-github-robot assigned janetkuo and 0xmichalis May 11, 2017

k8s-github-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. release-note Denotes a PR that will be considered when it comes time to generate release notes. labels May 11, 2017

k82cn force-pushed the k8s_45628 branch from 6a9e16b to 84ce346 Compare May 11, 2017 11:28

k8s-github-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels May 11, 2017

0xmichalis reviewed May 11, 2017

View reviewed changes

k82cn force-pushed the k8s_45628 branch from 84ce346 to db21be8 Compare May 11, 2017 12:09

k8s-github-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels May 11, 2017

k82cn force-pushed the k8s_45628 branch 2 times, most recently from f78f79a to 8c8aa22 Compare May 11, 2017 12:34

janetkuo reviewed May 11, 2017

View reviewed changes

k82cn force-pushed the k8s_45628 branch from 8c8aa22 to 826ac49 Compare May 12, 2017 04:18

k8s-github-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels May 12, 2017

k82cn changed the title ~~[WIP] Checked node condition for DaemonSets when updating node.~~ Checked node condition for DaemonSets when updating node. May 12, 2017

janetkuo reviewed May 12, 2017

View reviewed changes

k82cn force-pushed the k8s_45628 branch from 826ac49 to 4d393ae Compare May 13, 2017 07:17

lukaszo reviewed May 14, 2017

View reviewed changes

k8s-ci-robot assigned lavalamp May 18, 2017

k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 22, 2017

k82cn force-pushed the k8s_45628 branch from dca3c41 to 8f93304 Compare May 23, 2017 01:58

k8s-github-robot removed lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels May 23, 2017

0xmichalis mentioned this pull request May 24, 2017

DaemonSet: resync daemonsets when node status has changed #46307

Closed

lavalamp reviewed May 24, 2017

View reviewed changes

0xmichalis mentioned this pull request May 25, 2017

Daemonset pods not getting deployed after failing certain number of times #46386

Closed

Checked node condition for DaemonSets when updating node.

75eb9b4

k82cn force-pushed the k8s_45628 branch from 8f93304 to 75eb9b4 Compare June 1, 2017 00:47

k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 1, 2017

k8s-ci-robot assigned lukaszo Jun 1, 2017

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 1, 2017

k8s-github-robot merged commit fb76746 into kubernetes:master Jun 1, 2017

k82cn deleted the k8s_45628 branch June 1, 2017 13:27

k8s-ci-robot added the sig/apps Categorizes an issue or PR as relevant to SIG Apps. label Sep 2, 2018

Checked node condition for DaemonSets when updating node. #45649

Checked node condition for DaemonSets when updating node. #45649

Conversation

k82cn commented May 11, 2017 • edited

k8s-reviewable commented May 11, 2017

0xmichalis commented May 11, 2017

lukaszo commented May 11, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

janetkuo commented May 11, 2017

k82cn commented May 12, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

k82cn commented May 13, 2017

k82cn commented May 13, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

janetkuo commented May 17, 2017

janetkuo commented May 18, 2017

k82cn commented May 18, 2017

k82cn commented May 23, 2017

k82cn commented May 24, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

k82cn May 31, 2017 • edited

Choose a reason for hiding this comment

janetkuo May 31, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

k82cn commented Jun 1, 2017

lukaszo commented Jun 1, 2017

k82cn commented Jun 1, 2017

k82cn commented Jun 1, 2017

lukaszo commented Jun 1, 2017

k8s-github-robot commented Jun 1, 2017

k82cn commented Jun 1, 2017

k8s-github-robot commented Jun 1, 2017

k82cn commented Sep 2, 2018

k82cn commented May 11, 2017 •

edited

k82cn May 31, 2017 •

edited

janetkuo May 31, 2017 •

edited